Slim the sync-rep queue lock (4-patch series for upstream) - #4
Open
vbp1 wants to merge 4 commits into
Open
Conversation
vbp1
force-pushed
the
feat/syncrep-lock-slimming
branch
4 times, most recently
from
August 16, 2026 17:58
215e1bf to
5b6debc
Compare
SyncRepWakeQueue() sets each released backend's latch while holding SyncRepLock exclusively. A latch is a kill() syscall whenever its proc is asleep, and at a high commit rate the walsender runs one of them per released commit inside the very section every committer lines up on. ProcArrayGroupClearXid() already wakes its batch only after ProcArrayLock is down, for the same reason. Collect the released procs into a list instead, and set their latches once the lock is released. The unlink, the write barrier and the state store stay under the lock: a waiter reads syncRepState without the lock and must never find itself completed while still on the queue. Nothing in the deferred loop can error out, so a released proc cannot be left completed but unlatched short of the process dying outright -- a window the in-lock SetLatch had as well. A proc that noticed its state on its own and moved on, even into a new wait, gets a spurious latch set, which every latch sleeper tolerates. The list is sized to MaxBackends and allocated once per releasing process. A proc waits in at most one queue, so one list of that size bounds a walk over all three.
SyncRepReleaseWaiters() takes SyncRepLock and only then walks the walsender slots to work out the synced write, flush and apply positions. That walk takes a spinlock per slot, allocates, and for a quorum set sorts the result, and every cycle of it is spent in the section every committer lines up on. The comment there conceded the work does not need the lock and kept it inside anyway, to guarantee the positions are newer than any previous execution of the routine used. That guarantee is not needed. The three sites that consume the positions each move lsn[] forward only when the new reading is ahead of the stored one, so positions gone stale while the lock was being taken release nobody and change nothing; a concurrent walsender that got further has already stored its own. Compute them before taking the lock, and leave without taking it at all when this walsender turns out not to be a sync standby.
ProcessStandbyReplyMessage() calls SyncRepReleaseWaiters() for every reply it processes, and several replies routinely sit in the walsender's socket together. Each of those calls takes SyncRepLock exclusively, so a batch of replies costs the committers one period of that lock apiece -- computed, for all but the last reply, from positions the next message in the same batch immediately makes stale. Have a reply only mark a release as pending, and run one release at the end of the drain. The positions in shared memory are the newest of the batch by then, so the single pass releases everything the individual passes would have. A deferred release must survive every way out of the drain, because the positions the reply already stored are valid whatever follows and the committers it acknowledged have no other process to wake them: - the standby's goodbye, an EOF, an invalid message type and an unexpected message type each run the pending release before leaving. A clean standby shutdown sends its final reply and the goodbye back to back, which makes that exit the routine one rather than the exotic one. - an error thrown while a later message in the same drain is parsed -- a torn message above all -- leaves through WalSndErrorCleanup(), which runs the pending release after the locks are dropped. The test makes both coincidences certain instead of likely. For the first, a paused standby holds a remote_apply committer in the queue, the walsender is held with SIGSTOP while the standby applies past the commit and shuts down, and the released walsender drains the final reply and the goodbye in one pass. For the second, an injection point right after a drained reply stands in for the torn message; it fires on every reply, so the walreceiver is held with SIGSTOP while replay proceeds from WAL already on standby disk, which makes the first reply after release the one carrying the apply position the committer waits for. Both halves fail without their fix.
…lock SyncRepWaitForLSN() runs on every commit that wrote WAL, and it takes SyncRepLock exclusively before it can find out whether there is anything to wait for. On a busy primary a large share of those commits find their LSN already acknowledged and queue for nothing, so the answer costs them a period of the lock every other committer is lining up on. Mirror lsn[] into an atomic watermark, written under SyncRepLock right after lsn[] itself, and read it before taking the lock. A watermark that already covers the commit's LSN says a valid quorum acknowledged it, which is exactly the answer the check under the lock gives. Both are only ever moved forward, so a read gone stale can send a committer to the slow path that would have exited, but never past a wait it owes. On platforms where pg_atomic_read_u64() is not a plain load the read is itself a compare-and-exchange, or a spinlock acquisition where 64-bit atomics are emulated. Whether the exit still pays for itself there is untested.
vbp1
force-pushed
the
feat/syncrep-lock-slimming
branch
from
August 16, 2026 18:05
5b6debc to
f064395
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The sync-rep lock work from
feat/parallel-replay(PG 18.4), re-cut on topof upstream master (PG 20devel) as a series meant for a CommitFest thread.
This PR is for review before it goes to pgsql-hackers; it is not meant to be
merged into the fork's master as a feature.
Why four commits
ab27ab7fbf5on the source branch bundled four independent changes withdifferent risk profiles, and community review would split them anyway. The
two follow-ups there (
28140d131e8,dc08fd44e8f) only exist to keep thedeferred-release mechanism correct on every exit, so they are folded into
the commit that introduces it rather than shipped as fixups of a patch that
was never posted.
Every commit builds and tests on its own, so a committer can take a prefix
of the series.
What the port needed
Upstream drift in
syncrep.cbetween 18.4 and 20devel is cosmetic --SyncRepReleaseWaiters()andSyncRepWakeQueue()are structurallyunchanged. Two spots in
walsender.cneeded hand work:WalSndShmemInit()became
(void *arg)under the new shared-memory registration API, and thePqMsg_Terminatecomment was reworded. Note that master has grown a secondSyncRepReleaseWaiters()call site, on config reload, which is a directcall outside the reply drain and is left alone.
The TAP test moved from the parallel-replay module to
src/test/recovery/t/056_syncrep_release.pl; its only tie to the pool wasone GUC line. That suite already installs
injection_pointsand exportsenable_injection_points.Prior work
0001 has been proposed before, in a different shape. Thomas Munro's
Latches vs lwlock contention
thread carried
0006-Use-SetLatches-for-synchronous-replication-wakeups.patch,which does the same thing through a general
SetLatches()facility forbatched, deferred latch setting. That thread ran 2022-2024; its
heavyweight-lock half was committed in November 2024 (
3c0fd64fec8andfriends).
SetLatches()itself was not, nor the sync-rep user of it, andthe sync-rep patch appears never to have had a review of its own -- master
still calls
SetLatch()underSyncRepLockinsyncrep.c.0001 uses a list local to
syncrep.cinstead: contained in one file, nonew infrastructure, and it does not have to settle the questions the
general facility raised about fixed-size buffers and allocation inside lock
code. If
SetLatches()is revived, 0001 should be dropped in favour of it.0004 is not the first lock-free read of sync-rep state in
SyncRepWaitForLSN().2e57790836c(Michael Paquier, "Fix race withsynchronous_standby_names at startup", April 2025) reads
WalSndCtl->sync_standbys_statuswithout the lock and returns early on it,with the same argument: the state moves one way only, so a stale read costs
an unnecessary acquisition and nothing else. 0004 extends that to the LSN,
which needs the atomics API rather than a volatile read because it is 64
bits wide.
Measurements
Two-host stand, 20devel master vs master + this series
Primary: 4-socket Xeon Platinum 8580, 240 threads, 2 TB RAM, NVMe. Standby:
2-socket Xeon Gold 5320, 104 threads, 1 TB RAM, NVMe. Dedicated
point-to-point 100 GbE, RTT 0.11 ms. pgbench scale 2000 / fillfactor 70, 750
clients,
-M prepared, TPC-B, 10-minute runs,synchronous_commit = on,fsyncandfull_page_writeson. Every point starts from a byte-identicalcopy of one template cluster with a fresh base backup for the standby. Three
interleaved pairs. Driver:
bench/run-stand.sh, raw results understand-2026-08-16/.Spread within a build is 0.7% (base) and 1.6% (patched); replication lag
stayed under a few ms throughout.
The mechanism is visible in
pg_stat_activity, sampled every 5 s. Note boththe queue lock and the wait for the standby's ack are named
SyncRep--only
wait_event_typeseparates them:LWLock/SyncRepsamplesIPC/SyncRepsamplesPer committed transaction that is 0.0997 lock-wait samples against 0.0516 --
halved, while 9% more transactions go through. On base the queue lock is the
fourth-largest wait on the primary; with the series it falls to sixth.
WALWriteandProcarrayGroupUpdategrow, which is the next bottlenecksurfacing rather than a regression.
Lock counters, single host
The stand shows the size of the win but not how it divides among the four
patches. For that, a pair of
-DLWLOCK_STATSbuilds on a 16-thread box,both servers on one host, scale 20 -- absolute numbers there mean nothing,
the counters do.
SyncRepLock traffic, 128 clients, 30 s, per committed transaction:
At 32 clients the same pair gives 37809 blocks against 362, a factor of 104.
Two things to read out of this. 0003 does what it claims: the walsender
takes the lock roughly half as often, one pass per drained batch instead of
one per reply. And 0004 barely fires -- it saves a backend 1.6-2.3% of its
acquisitions even at 128 clients. That is inherent to the wait mode: with
synchronous_commit = onthe standby cannot have flushed your ownjust-written commit record before you reach the wait, so the fast exit only
catches the commits whose LSN a later transaction's acknowledgement happened
to cover. 0004 is the weakest-earning patch of the four on this workload.
Single-client latency, 60 s, four interleaved pairs: base 0.834, 0.759,
0.744, 0.704 ms; patched 0.709, 0.757, 0.827, 0.755 ms. Means 0.760 against
0.762 ms. This is the measurement 0003 owes: with one reply per drain there
is no batch to coalesce, and deferring the release to the end of the drain
does not delay the waiter.
Verification
make check-worldgreen on the tip of the series.stepwise.
exit safeguards removed, both subtests fail; with them, both pass.
pgindentandpgperltidyproduced no changes.Getting the patch files
--baserecords the commit the series applies to, which the CommitFestrobot reads. All four files go as attachments to a single message on one
pgsql-hackers thread; a reroll resends the whole series with
-v 2.Open questions for review
lsn_published[]outside the lock. A second opinion was takenon the safety argument and could not refute it: both that field and
lsn[]only ever move forward under the lock, so a stale read can onlycost a needless acquisition, never skip a wait that is owed. What remains
open is whether it earns its keep. It is the one patch that grows
WalSndCtlData, and on platforms wherepg_atomic_read_u64()is not aplain load it turns a read into a compare-and-exchange -- or a spinlock
where 64-bit atomics are emulated -- on a shared cache line, on every
commit, to save the 2% of acquisitions measured above. A reviewer may
reasonably want it dropped from the series or gated.
MaxBackends-sized wakelist per releasing process. Anupstream reviewer may want that sized or shaped differently.